Skip to content

Bound the rate-limit retry budget at 30 minutes - #284

Merged
MichaelGHSeg merged 8 commits into
masterfrom
retry-budget-bounds
Sep 26, 2026
Merged

MichaelGHSeg merged 8 commits into
masterfrom
retry-budget-bounds

Conversation

@MichaelGHSeg

@MichaelGHSeg MichaelGHSeg commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Bounds the rate-limit retry path, which was limited only by a 12 hour duration.

Defaults

  • max_rate_limit_duration is 30 minutes, was 12 hours. It must stay well above the 300s rate_limit_retry_after_cap: at parity a single maximal wait consumes the whole budget, the episode makes one attempt, and the cap stops binding entirely because whatever is left of the budget is always the smaller term. A spec asserts that relationship and fails with "budget 300s against a 300s cap leaves no room to retry".
  • rate_limit_retry_after_cap stays at 300s.

Behaviour

  • A Retry-After that will not fit in what is left of the budget ends the episode rather than being shortened. Shortening resumes inside the window the server named — one it has already declined to serve — and the budget is spent by then. max_total_backoff_duration works the same way.
  • next_rate_limit_delay takes one clock reading for the episode start, the budget test and the delay. The ||= initialising @rate_limit_start_time used to take its own, leaving remaining a hair under the budget on an episode's first response — enough to lose an exact comparison against the cap, which failed two specs on any clock with sub-microsecond resolution. Measured 653 failures in 20,000 on macOS at 1µs; Linux CI resolution makes it near-certain.

Notes for review

  • The spec guarding the single reading asserts the reading count, not the returned value: a second reading past the budget makes the method return nil, which any "never negative" assertion accepts, so only the count separates the fix from the defect.
  • Backoff pacing changes from 2.5.0 (base 100ms → 500ms, ceiling 10s → 60s, multiplier 1.5 → 2) carry their own upgrade note.
  • The 12 hour default was never released — 2.5.0 predates this work.

The 12 hour default was a backstop on the assumption a retry count would
stop us reaching it. Rate-limited attempts are deliberately uncounted, so
it was the operative limit instead. With one worker thread that meant a
stuck batch stalled all delivery for half a day, filled the 10,000-message
queue, and blocked flush for the same period.

Five minutes matches the counted path's ~4 minute worst case.

rate_limit_retry_after_cap drops to 60s: at 300s it equalled the whole
budget, so one sleep consumed it and the rate-limit path gave a single
attempt.

The delay is also clamped to the remaining budget, since the elapsed check
runs before the wait.

The RetryBudget spec helper now takes the shipped defaults from
Defaults::Request rather than restating them, so a spec cannot pass against
numbers the library no longer uses — which is exactly what happened when
the cap moved and the helper kept its own 300.

214 examples, rubocop clean, 61-test e2e suite passes.
Three problems. The notes described changes between states that never
shipped, so a customer read that a default moved from 12 hours to 5 minutes
when only the 5 minutes was ever released. They referred to other SDKs,
which means nothing to someone reading one library's notes. And they had
accumulated over several passes into contradictions — Retry-After was
documented as capped at both 300s and 60s, and the rate-limit budget as
both 12 hours and 5 minutes.

Rewritten to describe the behaviour this version has, in a consistent
structure: upgrade notes that need action first, then retry handling, then
everything else. Entries covering fixes to code that has not shipped are
dropped, since there is nothing for a reader to compare against.
The budget test and the remaining-time calculation each took their own
reading, so the budget could expire between them. That yields a negative
remaining and a negative delay, and Kernel#sleep raises ArgumentError on a
negative interval rather than returning immediately — so the worker thread
would die rather than the episode ending.

One reading now serves both, and the guard tests the remaining time
directly. The condition is equivalent: remaining <= 0 is elapsed >= limit.

Found while reviewing my own change. php samples once and is unaffected;
python reuses one sample and guards on a positive wait; go and C# express
the overshoot as a timer or a timestamp, where a negative is harmless.

Two of the three new specs are worth noting as nearly useless: asserting
the boundary passes against the two-reading version as well, because the
defect is the gap between readings rather than the boundary itself. The
third stubs the clock so the later reading falls outside the budget, and
that one does fail against the old code, reporting the negative it would
have handed to sleep.

217 examples, rubocop clean, 61-test e2e suite passes.
Applying the team convention to my own work from today. The comments
explaining these changes had accumulated into potted histories: why a value
had been twelve hours, what a test used to assert, which path used to be
unreachable. Six months from now none of that resolves to anything — the
diff and the commit messages hold it, and the comment should say why the
code is the way it is.

What stayed is what a maintainer would undo without it: that Kernel#sleep
raises on a negative interval, that Thread#wakeup only interrupts a sleep
already in progress, that OkHttp's reads are governed by SO_TIMEOUT so an
interrupt does not reach them, and that inverting one assertion would make
the duration budget unreachable again.

Comments only, no behaviour change.
Capping at 60s meant waiting less than the server asked for, which does
not make the next attempt more likely to succeed — it just sends more
requests at something already rate-limiting us. Against a Retry-After of
180s inside a 5 minute budget it turns 3 requests into 6; against 300s it
turns 2 into 6.

The cap is a guard against an absurd header, not a second budget. How long
we keep trying is max_rate_limit_duration's job, and the clamp to the
remaining budget already stops a single wait running past it, so the cap
now rarely binds at all.

It also bought nothing for the client this was partly aimed at: with no
background thread, a shorter cap turns one long wait into several short
ones for the same total blocking time and more requests.

Tests that pinned 60 are updated, and each SDK gains one asserting that a
Retry-After inside the cap is used as given rather than shortened.
The ||= initialising @rate_limit_start_time took its own reading, so the
budget test and the delay were computed from two. On an episode's first
response that left remaining a hair under max_rate_limit_duration, and
since the cap and the budget are both 300 it is remaining that wins the
min -- so a large Retry-After returned 299.99999928 rather than 300.

Two exact-equality assertions land on that value, one of them in
transport_spec and untouched by this branch. They pass on macOS, whose
CLOCK_MONOTONIC resolution is 1us and where consecutive readings usually
collide, and fail on Linux CI, where they do not. Measured here: 653
failures in 20000 constructions before, 0 after.

The spec written to guard this could not catch it. It stubbed three
readings to drive the delay negative, but a second reading past the budget
makes the method return nil, which its "never negative" assertion accepts.
It now asserts the reading count, which is the property in question and
fails deterministically: expected 1 time, received 2.

218 examples, 0 failures. Rubocop's 27 offences are all in e2e-cli/main.rb
and predate this branch.
The budget and the Retry-After cap were both 300s, and at parity the
rate-limit path degenerates. A response with no usable Retry-After waits
the cap by default, the elapsed check runs before the wait, so that one
wait spends the whole budget and the batch is dropped having been tried
once. A legitimate Retry-After of 300 does the same. The cap also stops
binding: whatever is left of the budget is always the smaller term, so the
cap can never be the value that clamps.

Thirty minutes restores the relationship the two knobs are meant to have —
the cap bounds one wait, the budget bounds the episode — and leaves room
for several attempts. It costs nothing in normal operation, since the
budget only binds when the server has been rate-limiting us for a long
time, and in that case keeping the data is the point.
Review point, and a fair one: shortening a Retry-After to fit the remaining
budget sends the next request before the time the server named, which it has
already said it will not serve, and the budget is spent by then so it would be
the final attempt either way. The clamp bought one guaranteed-refused request per
episode at a server already rate-limiting us. Both paths now give up at that
point instead.

next_backoff_delay gets the same rule, and with it the single clock reading the
rate-limit path already had -- elapsed? took its own, which was harmless while
nothing was derived from the difference and would not have been once a fit check
depends on it.

Added the spec pinning budget > cap with room for two waits. go, python and C#
each got one; ruby's own comment states the invariant most precisely of the six
and nothing tested it, so reverting the default to parity left the suite green.
It now fails with "budget 300s against a 300s cap leaves no room to retry".

220 examples, rubocop unchanged (its 27 offences are all in e2e-cli/main.rb and
predate this branch).
@MichaelGHSeg MichaelGHSeg changed the title Bound the rate-limit retry budget at 5 minutes Bound the rate-limit retry budget at 30 minutes Sep 25, 2026
@MichaelGHSeg
MichaelGHSeg merged commit 2027ce5 into master Sep 26, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants